Skip to content

bpo-38811: Check for presence of os.link method in pathlib#17170

Merged
serhiy-storchaka merged 2 commits into
python:masterfrom
tohojo:bpo-38811
Nov 17, 2019
Merged

bpo-38811: Check for presence of os.link method in pathlib#17170
serhiy-storchaka merged 2 commits into
python:masterfrom
tohojo:bpo-38811

Conversation

@tohojo

@tohojo tohojo commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

Commit 6b5b013 ("bpo-26978: Implement pathlib.Path.link_to (Using
os.link) (GH-12990)") introduced a new link_to method in pathlib. However,
this makes pathlib crash when the 'os' module is missing a 'link' method.

Fix this by checking for the presence of the 'link' method on pathlib
module import, and if it's not present, turn it into a runtime error like
those emitted when there is no lchmod() or symlink().

Signed-off-by: Toke Høiland-Jørgensen toke@redhat.com

https://bugs.python.org/issue38811

@the-knights-who-say-ni

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@tohojo

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@tohojo

tohojo commented Nov 15, 2019

Copy link
Copy Markdown
Contributor Author

I assume Red Hat already signed the CLA. How do I go about having that registered in the system?

@cool-RR cool-RR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@cool-RR

cool-RR commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

Is there a way to add testing for this code?

@cool-RR

cool-RR commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

Out of curiosity, which system did you try it on that didn't have link?

@tohojo

tohojo commented Nov 15, 2019 via email

Copy link
Copy Markdown
Contributor Author

@cool-RR

cool-RR commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

Dunno? Looking at the other methods that are conditional in pathlib, I'm
not so sure: symlink just has a "skip_unless_symlink" decorator, and
lchmod() has no test at all.

I understand, so there's some precedent for leaving it untested, though it wouldn't be a bad idea to add testing both for this feature and the other methods, both in cases where they're supported and not.

@tohojo

tohojo commented Nov 15, 2019 via email

Copy link
Copy Markdown
Contributor Author

@tohojo

tohojo commented Nov 15, 2019 via email

Copy link
Copy Markdown
Contributor Author

@cool-RR

cool-RR commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

Whatever Travis CI is using: https://travis-ci.org/tohojo/flent/jobs/611950249

That is curious indeed, especially since Linux is mentioned there. @bittner do you have an idea which OS Travis is running on?

But how? The pathlib definition is done at import, so it's not quite as
simple as simply mock()'ing out the link method in os, is it? It would
take quite some refactoring of all the pathlib tests, no?

I agree it's difficult and you don't have to take it on. One simple idea would be to run a test only in cases where os.link isn't defined, and then have that test call Path.link_to and expect the NotImplementedError.

@tohojo

tohojo commented Nov 15, 2019 via email

Copy link
Copy Markdown
Contributor Author

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a bit of cleanup here:

Comment thread Lib/pathlib.py Outdated
Comment thread Lib/test/test_pathlib.py Outdated
Comment thread Misc/NEWS.d/next/Library/2019-11-15-18-06-04.bpo-38811.AmdQ6M.rst Outdated
@brandtbucher

Copy link
Copy Markdown
Member

Also, the CLA is linked to your BPO account, so you'll need to follow the bot's instructions to sign it.

@brandtbucher brandtbucher added the type-bug An unexpected behavior, bug, or error label Nov 15, 2019
@brandtbucher

Copy link
Copy Markdown
Member

I wonder if Travis intentionally disables symlinks, as a security measure.

@brandtbucher

Copy link
Copy Markdown
Member

Also, welcome to CPython @tohojo!

@tohojo

tohojo commented Nov 15, 2019

Copy link
Copy Markdown
Contributor Author

Thanks! I'll see if I can figure out the correct way to handle the CLA :)

@brandtbucher

Copy link
Copy Markdown
Member

Here's the link again.

@tohojo

tohojo commented Nov 15, 2019

Copy link
Copy Markdown
Contributor Author

Yeah, the trouble with that is that I can't really sign that since my corporate overlord already owns the rights to everything I do; so they will have to sign it. "They" being Red Hat, I'm assuming they already did, I just need to figure out how to get that into the PSF system. I seem to recall there being something about this on our intranet, but that is just giving me "service unavailable" errors right now, so I guess I'll have to try again a bit later :)

Comment thread Lib/pathlib.py Outdated
@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Commit 6b5b013 ("bpo-26978: Implement pathlib.Path.link_to (Using
os.link) (pythonGH-12990)") introduced a new link_to method in pathlib. However,
this makes pathlib crash when the 'os' module is missing a 'link' method.

Fix this by checking for the presence of the 'link' method on pathlib
module import, and if it's not present, turn it into a runtime error like
those emitted when there is no lchmod() or symlink().

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
This adds a test case to test_pathlib.py to test the NotImplementedError
raised when symlink support is not present on the system. Also add the
missing @staticmethod decorator to the function definition in pathlib.py
for that error path.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
@tohojo

tohojo commented Nov 16, 2019

Copy link
Copy Markdown
Contributor Author

Oh, BTW, is there an automatic process to get this into 3.8.1, or should I open another pull request against the 3.8 branch?

@corona10

Copy link
Copy Markdown
Member

@tohojo I add backport label for 3.8. Bot will auto-create the PR for 3.8!

@tohojo

tohojo commented Nov 17, 2019 via email

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka
serhiy-storchaka merged commit 111772f into python:master Nov 17, 2019
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @tohojo for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot

Copy link
Copy Markdown

GH-17204 is a backport of this pull request to the 3.8 branch.

@bedevere-bot

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows7 SP1 3.x has failed when building commit 111772f.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/40/builds/3413) and take a look at the build logs.
  4. Check if the failure is related to this commit (111772f) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/40/builds/3413

Failed tests:

  • test_pathlib
  • test_asyncio

Failed subtests:

  • test_symlink_to_not_implemented - test.test_pathlib.PathTest
  • test_huge_content - test.test_asyncio.test_sock_lowlevel.SelectEventLoopTests
  • test_symlink_to_not_implemented - test.test_pathlib.WindowsPathTest

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

385 tests OK.

10 slowest tests:

  • test_multiprocessing_spawn: 2 min 54 sec
  • test_tools: 2 min
  • test_tokenize: 1 min 53 sec
  • test_concurrent_futures: 1 min 35 sec
  • test_capi: 1 min 28 sec
  • test_mmap: 1 min 20 sec
  • test_lib2to3: 1 min 17 sec
  • test_asyncio: 1 min 11 sec
  • test_unicodedata: 1 min 4 sec
  • test_io: 57.6 sec

1 test failed:
test_pathlib

33 tests skipped:
test_curses test_dbm_gnu test_dbm_ndbm test_devpoll test_epoll
test_fcntl test_fork1 test_gdb test_grp test_ioctl test_kqueue
test_multiprocessing_fork test_multiprocessing_forkserver test_nis
test_openpty test_ossaudiodev test_pipes test_poll test_posix
test_pty test_pwd test_readline test_resource test_spwd
test_syslog test_threadsignals test_tix test_tk test_ttk_guionly
test_wait3 test_wait4 test_xxtestfuzz test_zipfile64

2 re-run tests:
test_asyncio test_pathlib

Total duration: 11 min 29 sec

Click to see traceback logs
Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\proactor_events.py", line 768, in _loop_self_reading
    f.result()  # may raise
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\windows_events.py", line 808, in _poll
    value = callback(transferred, key, ov)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\windows_events.py", line 457, in finish_recv
    raise ConnectionResetError(*exc.args)
ConnectionResetError: [WinError 995] The I/O operation has been aborted because of either a thread exit or an application request


Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\windows_events.py", line 66, in _cancel_overlapped
    self._ov.cancel()
OSError: [WinError 6] The handle is invalid


Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\proactor_events.py", line 768, in _loop_self_reading
    f.result()  # may raise
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\windows_events.py", line 808, in _poll
    value = callback(transferred, key, ov)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\windows_events.py", line 457, in finish_recv
    raise ConnectionResetError(*exc.args)
ConnectionResetError: [WinError 995] The I/O operation has been aborted because of either a thread exit or an application request
k


Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_asyncio\test_sock_lowlevel.py", line 170, in test_huge_content
    self.loop.run_until_complete(
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\base_events.py", line 634, in run_until_complete
    return future.result()
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_asyncio\test_sock_lowlevel.py", line 157, in _basetest_huge_content
    data = await self.loop.sock_recv(sock, DATA_SIZE)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\selector_events.py", line 362, in sock_recv
    return await fut
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\selector_events.py", line 373, in _sock_recv
    data = sock.recv(n)
ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host


Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\windows_events.py", line 453, in finish_recv
    return ov.getresult()
OSError: [WinError 995] The I/O operation has been aborted because of either a thread exit or an application request


Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\asyncio\windows_events.py", line 66, in _cancel_overlapped
    self._ov.cancel()
OSError: [WinError 6] The handle is invalid
k


Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_pathlib.py", line 2031, in test_symlink_to_not_implemented
    link.symlink_to(target)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\pathlib.py", line 1395, in symlink_to
    self._accessor.symlink(target, self, target_is_directory)
OSError: [WinError 1314] A required privilege is not held by the client: 'C:\\buildbot.python.org\\3.x.kloth-win64\\build\\build\\test_python_1660\\@test_1660_tmp\\fileA' -> 'C:\\buildbot.python.org\\3.x.kloth-win64\\build\\build\\test_python_1660\\@test_1660_tmp\\dirA\\linkAA'


Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_pathlib.py", line 2031, in test_symlink_to_not_implemented
    link.symlink_to(target)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\pathlib.py", line 1395, in symlink_to
    self._accessor.symlink(target, self, target_is_directory)
OSError: [WinError 1314] A required privilege is not held by the client: 'C:\\buildbot.python.org\\3.x.kloth-win64\\build\\build\\test_python_1660\\test_python_worker_6236\\@test_6236_tmp\\fileA' -> 'C:\\buildbot.python.org\\3.x.kloth-win64\\build\\build\\test_python_1660\\test_python_worker_6236\\@test_6236_tmp\\dirA\\linkAA'

@brandtbucher

brandtbucher commented Nov 17, 2019

Copy link
Copy Markdown
Member

@serhiy-storchaka This was merged without a signed CLA...

@bedevere-bot

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Windows7 3.x has failed when building commit 111772f.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/58/builds/3257) and take a look at the build logs.
  4. Check if the failure is related to this commit (111772f) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/58/builds/3257

Failed tests:

  • test_pathlib

Failed subtests:

  • test_symlink_to_not_implemented - test.test_pathlib.PathTest
  • test_symlink_to_not_implemented - test.test_pathlib.WindowsPathTest

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

388 tests OK.

10 slowest tests:

  • test_multiprocessing_spawn: 20 min 27 sec
  • test_zipfile: 13 min
  • test_concurrent_futures: 8 min 29 sec
  • test_regrtest: 6 min 26 sec
  • test_tarfile: 5 min 19 sec
  • test_asyncio: 5 min 12 sec
  • test_pickle: 4 min 11 sec
  • test_capi: 4 min 4 sec
  • test_venv: 3 min 57 sec
  • test_distutils: 3 min 54 sec

1 test failed:
test_pathlib

30 tests skipped:
test_curses test_dbm_gnu test_dbm_ndbm test_devpoll test_epoll
test_fcntl test_fork1 test_gdb test_grp test_ioctl test_kqueue
test_multiprocessing_fork test_multiprocessing_forkserver test_nis
test_openpty test_ossaudiodev test_pipes test_poll test_posix
test_pty test_pwd test_readline test_resource test_spwd
test_syslog test_threadsignals test_wait3 test_wait4
test_xxtestfuzz test_zipfile64

1 re-run test:
test_pathlib

Total duration: 1 hour 32 min

Click to see traceback logs
Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_pathlib.py", line 2031, in test_symlink_to_not_implemented
    link.symlink_to(target)
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\pathlib.py", line 1395, in symlink_to
    self._accessor.symlink(target, self, target_is_directory)
OSError: [WinError 1314] A required privilege is not held by the client: 'D:\\cygwin\\home\\db3l\\buildarea\\3.x.bolen-windows7\\build\\build\\test_python_848\\@test_848_tmp\\fileA' -> 'D:\\cygwin\\home\\db3l\\buildarea\\3.x.bolen-windows7\\build\\build\\test_python_848\\@test_848_tmp\\dirA\\linkAA'


Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_pathlib.py", line 2031, in test_symlink_to_not_implemented
    link.symlink_to(target)
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\pathlib.py", line 1395, in symlink_to
    self._accessor.symlink(target, self, target_is_directory)
OSError: [WinError 1314] A required privilege is not held by the client: 'D:\\cygwin\\home\\db3l\\buildarea\\3.x.bolen-windows7\\build\\build\\test_python_848\\test_python_worker_2032\\@test_2032_tmp\\fileA' -> 'D:\\cygwin\\home\\db3l\\buildarea\\3.x.bolen-windows7\\build\\build\\test_python_848\\test_python_worker_2032\\@test_2032_tmp\\dirA\\linkAA'

vstinner added a commit that referenced this pull request Nov 18, 2019
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
…-17170)

Fix also the Path.symplink() method implementation for the case when
symlinks are not supported.
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
…-17170)

Fix also the Path.symplink() method implementation for the case when
symlinks are not supported.
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants